feat: default, pinned and required filters in explore - #9698
AdityaHegde wants to merge 13 commits into
Conversation
nishantmonu51
added
Type:Feature
|
@codex: review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c5605f14b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
Two of these do not sit on a line in the diff, so they are here, plus one note.
The frontend half is superseded by #9746 on main. The merge base is f4ee385023 (2026-07-23), and git merge-tree reports content conflicts in 11 files plus modify/delete conflicts in canvas/stores/filter-manager.ts, dashboards/stores/Filters.ts, filters/FilterChipsReadOnly.svelte, filters/ExploreFilterChipsReadOnly.svelte and filters/test/render-filter-component.ts, all deleted by bc0c59c965. That commit already ships YAMLConfigProvider.pinnedFilters/requiredFilters, JoinerFilterManager, MissingRequiredFiltersMessage.svelte, filters/utils.ts:getMissingRequiredFilters and pinned/required rendering in DimensionFilter.svelte/MeasureFilter.svelte, and it leaves // TODO: once we have this support for explore at DashboardConfigProvider.svelte.ts:39 waiting for exactly this PR's backend fields. YAMLOnlyExploreState, required/required-filters.ts, RequiredFiltersMessage.svelte and the Filters.svelte rewrite are therefore a second, incompatible implementation of what main already has. The backend half survives a rebase, except that parse_explore.go conflicts because #9680 extracted the inline Defaults struct into ExploreDefaultsYAML.
required_filters, pinned_filters and the default where bypass field-access security. applyExploreSecurity strips inaccessible names from spec.Dimensions, spec.Measures, DefaultPreset.Dimensions and DefaultPreset.Measures, but returns DefaultPreset.RequiredFilters, PinnedFilters and Where untouched (runtime/resources.go:320-336). For a viewer whose policy hides dimension d: with required_filters: [d], d is absent from allDimensions, so getDimensionFilters skips the synthetic chip and getMissingRequiredFilters falls through to the "no entry" branch (required-filters.ts:37), producing a permanent "select a value for d" block that names the restricted field and can never be satisfied. With filter: "d = 'x'", the initial whereFilter is compiled by SQLForExpression(where, nil, false, true) and LookupDimension(name, visible=true) returns ErrForbidden (runtime/metricsview/ast.go:702), so every query on the dashboard errors. The security rewrite needs to drop restricted names from all three preset fields and prune Where sub-expressions that reference them.
Note: saveExploreDefaults swallows a queryServiceConvertExpressionToMetricsSQL failure and still calls doc.set("defaults", defaults), so a transient error while saving an unrelated change silently drops a configured filter: from the YAML (save-explore-defaults.ts:75-83, :98).
| repeated string pinned_filters = 40; | ||
| // Array of dimension or measure names that must have a value before the explore can render. | ||
| // Required filters are implicitly pinned. | ||
| repeated string required_filters = 41; |
There was a problem hiding this comment.
pinned_filters = 40 collides with optional string ephemeral_measures = 40, which #9855 (e5da790c5d) added to ExplorePreset on main. After a rebase the message declares two fields numbered 40, buf rejects the file, and resources.pb.go (already a content conflict in merge-tree) cannot be regenerated. Both new fields need renumbering.
| $: if (exploreSpec) { | ||
| resolvedYamlOnlyState.sync(exploreSpec); | ||
| } |
There was a problem hiding this comment.
This runs on every exploreSpec change and replaces both in-memory arrays with the spec values, discarding unsaved pin/required toggles. Autosave is forced on in the viz editor (ExploreWorkspace.svelte:188: autoSave={selectedView === "viz" || $autoSave}), so toggling "pinned" on a chip and then editing something unrelated such as a display name writes the file, the reconciler produces a new validSpec, and the toggle is silently reverted before the user reaches "Save as default".
| [metricsViewName, dim], | ||
| ]), | ||
| pinned: false, | ||
| pinned: pinnedFilters?.has(ident), |
There was a problem hiding this comment.
The LIKE/NLIKE branch sets pinned but never required, unlike the IN/NIN branch at :197-198. DimensionFilter.svelte:461 gates removal on !curPinned && !required, so a dimension listed only in required_filters that currently carries a contains filter renders as removable; removing it re-adds an empty required chip via getDimensionFilters and the dashboard is then blocked by RequiredFiltersMessage.
|
Closing since this is quite a bit stale and will be even so after time controls unification. |
2 participants
Adds support for default, pinned and required filters similar to canvas. Adds a separate
YAMLOnlyExploreStatethat doesnt interact with url, it is updated during editing but readonly during preview. Moves the explore defaults button to the topright similar to canvas.Closes APP-614
Checklist: